commit message nit: Let's try to use more brief wording, say:

model [nfc]: Clarify how to add updatable fields to `ZulipStream`

or

model [nfc]: Clarify when to add non-final fields to `ZulipStream`

Also, I believe this is closely related to the previous commit
api: Add stream/update event
So, most probably this will get squashed into it during maintainer or integration review.

I think this contains all the information it needs. (And it's more specific than "int": an API value for this enum can be one of a few ints, not any int.)

(Possibly the existing fromRawString methods on other enums, like Emojiset, could have been better named fromApiValue as well.)

…Ah, but see a comment elsewhere about how the null case doesn't seem necessary; so I think this could just be fromApiValue, not fromApiValueOrNull.

This is a good change and I'd go a step further: the new name covers all the information in the doc, so that would can leave the doc out.

Do we expect to get an event with property: 'stream_post_policy' and a missing or null value? I don't think we do. In the API doc for the event and for the register response, it doesn't say we should expect missing or null, except from servers pre-3.0, and we don't support those servers.

With this change, ChannelPostPolicy.fromInt can also tightened to expect an int instead of int?.

Do we expect to get an event with property: 'stream_post_policy' and a missing or null value?

Yeah, I don't think so. But the reason I added int? instead of int is that there is ChannelPostPolicy.unknown for apiValue: null. I think it was added for backward-compatibility with server-3 at that time.

Hmm. @gnprice, does that seem right?

I agree we should expect "property": "stream_post_policy" to mean that value will be an int.

The value ChannelPostPolicy.unknown is there mainly for forward compatibility, not backward: it's "unknown" because it represents a hypothetical future server sending a value like 5 or 6 instead of 1 or 2 or 3 or 4, in which case that 5 or 6 would represent some new meaning that this client version doesn't know about.

In general zulip-flutter only supports down to Zulip Server 4:
https://github.com/zulip/zulip-flutter#editing-api-types
so we don't have any compatibility logic that's meant for server versions 3 or earlier (and stream_post_policy was introduced in Zulip Server 3). If the field were missing in some versions we still support, then we'd represent that like

  ChannelPostPolicy? channelPostPolicy;

i.e. with null to represent missing.

(Rereading the code, I think we may not have actually correctly accomplished that forward-compatibility — I think we might just throw on an unexpected value like 5. For an example I believe does work correctly, see MessageFlag.)

alwaysCreate: true and fieldRename: FieldRename.kebab aren't passed to the JsonEnum annotation, so this comment is wrong. 🙂 Looks like it was copy-pasted from somewhere else.

_$ChannelPostPolicyEnumMap is created because ChannelPostPolicy is used as a field in a class (ZulipStream) that's annotated with JsonSerializable; see the dartdoc on JsonEnum.alwaysCreate:

  /// If `true`, `_$[enum name]EnumMap` is generated for in library containing
  /// the `enum`, even if the `enum` is not used as a field in a class annotated
  /// with [JsonSerializable].
  ///
  /// The default, `false`, means no extra helpers are generated for this `enum`
  /// unless it is used by a class annotated with [JsonSerializable].
  final bool alwaysCreate;

Probably I'd just delete the comment.

We use asserts when there's an invariant we're managing purely in our own code. But here, the assert could fail because of unexpected server behavior. When we want to guard against unexpected server behavior, we don't use asserts. In production, asserts are ignored, and also, arguments to them aren't evaluated: https://dart.dev/language/error-handling#assert . We might instead throw an error, or just give up on the thing we were trying to do and move on, usually with a // TODO(log).

Here, though, we don't need the channel name on the event, do we? We can just look it up on the ZulipStream instance in our data structures. It seems potentially nice to avoid depending on the channel name in the event, to give the API more freedom to deprecate/remove that field as a cleanup.

I think the assert is actually for making sure our two local copies in streams and streamsByName are the same. The same assert is used in ChannelDeleteEvent.

streamName is actually useful when updating streamsByName when the event is about updating the channel name:

case ChannelPropertyName.name:
  channel.name = event.value as String;
  streamsByName.remove(streamName); 
  streamsByName[channel.name] = channel;

But if we really want to prevent getting streamName from the event, we can get it by storing channel.name in a temporary variable. Should I go with the latter option?

I think the assert is actually for making sure our two local copies in streams and streamsByName are the same.

I see. Then I guess here, and also where we handle ChannelDeleteEvent, it should be possible to do a targeted assert for that internal data-structure check, using nothing but the data we have locally. We can look up the channel in our streams map, get its name, look up the name in our streamsByName map, and check that the ZulipStream there matches the ZulipStream in streams.

And for the ways that unexpected server behavior can mislead our app, we can add checks that do run in production. Those ways, I think, are:

• The channel ID in the event isn't found in our data structures (our streams map). Let's early-return in this case, as in your PR, but add a // TODO(log).
• The event's idea of the current channel name doesn't match our data structures. We can // TODO(log) in this case, but we can probably cheerfully continue handling the event, just ignoring the name in the event to keep our streams and streamsByName consistent with each other. Since the server is giving unexpected behavior, I think there's no strong reason to trust the name in the event more than the name in our data structures. But the TODO(log) could be helpful for catching bugs where our expectations are misaligned with server behavior.

Could you send a revision with a separate commit that does this for ChannelDeleteEvent, and also does it here for ChannelUpdateEvent?

But if we really want to prevent getting streamName from the event, we can get it by storing channel.name in a temporary variable. Should I go with the latter option?

Yeah, this seems helpful. I've just checked, and the legacy app doesn't use the channel name on this event. Since it's possible to avoid using it now (just by using the name from the entry our streams map), let's avoid it, and that'll make it easy if the API wants to deprecate/remove the field in the future.

This looks to me like the kind of assert we do use: checking that our own data structures are consistent with each other.

I think it's probably OK to accept null for this one, but from reading the API docs (including the register-queue doc), I'm not sure it's ever expected, and what it would mean. The field on ZulipStream is nullable, but I think that's only for backwards compatibility with older servers that don't have the field:

  // TODO(server-6): `canRemoveSubscribersGroupId` added in FL 142
  // TODO(server-8): in FL 197 renamed to `canRemoveSubscribersGroup`
  @JsonKey(readValue: _readCanRemoveSubscribersGroup)
  int? canRemoveSubscribersGroup;

And if we're in this case, then surely we're not on one of those older servers.

So, probably nothing to do here, and no harm in being a little more permissive with int? instead of int, I think, as long as the field on ZulipStream remains nullable.

If adding this to the 'added by events' test, maybe good to rename the test to 'added/updated by events'.

nit:

• that aligns it with the closely-related fromApiValue
• "by int value" isn't quite accurate — one of these keys is null, not an int

This feels like it gets rather more complicated to follow (as well as longer — 6 lines to 13 lines). There's now multiple stream IDs going around, as well as multiple stream names.

So I'm not convinced this is a helpful change. (/cc @chrisbobbe from the previous comment #880 (comment) above)

This case doesn't otherwise do anything with subscriptions, so this assert can be left out.

This also doesn't do anything with streamsByName except in the ChannelPropertyName.name case. So this assert can be moved into that case.

Then so can the streamName local.

And then I think we don't use the streamId local at all, except in doing this second streams lookup. (Which would be clearer as just saying stream, optionally with a separate assert that stream.streamId matches event.streamId — as is, if these data structures get corrupted by a bug somewhere else in our code, there's nothing local in this code to assure us that when we looked up event.streamId in streams we got a ZulipStream whose stream ID is actually event.streamId.)

This also doesn't do anything with streamsByName except in the ChannelPropertyName.name case. So this assert can be moved into that case.

Moved it to its specific case. But this assert in here actually helped me spot a bug in one of my previous revisions of this PR (actually when working on #886 which is stacked on top of this PR). In that revision, I forgot to include:

case ChannelPropertyName.name:
  stream.name = event.value as String;
+  streamsByName.remove(streamName);
+  streamsByName[stream.name] = stream;

So when first the channel name is changed and then some other property, i.e., channelPostPolicy, the assert would fail, simply because streamsByName was not updated accordingly. And now if we somehow have a bug where we don't update streamsByName, then the assert in the new location will not tell us about it. But now that it is covered in the tests, I think we're good to go. 🙂

Yeah, that makes sense. I do think checking for that sort of inconsistency is useful. Two places we could do that that I think would work well (and which could coexist):

• We could use the same pattern as the checkInvariants function in test/model/message_list_test.dart: we'd write a function that scrubs the entire data structure and checks every aspect for consistency, and then have all the test cases in test/model/channel_test.dart arrange to call that function after every step they take. This is very thoroughly effective, for every situation that's exercised in those tests (even if the individual test doesn't think to check that aspect).
• We could arrange so that in the live app (in debug mode), every access to a channel or subscription in ChannelStore asserts that _streams and _streamsByName and _subscriptions are consistent with each other about that particular channel. In that case the asserts should get factored into a single method (with "debug" at the start of its name), which we can call from various places in lib/model/channel.dart. For accesses made from the rest of the app, we can extend MapView (perhaps better yet, UnmodifiableMapView?) and add the asserts to operator[].

But I'm also content to leave those aside for now.

see other examples for the format; and in server versions 6 and 7 this does exist (in fact it's new in 6), it's version 8 where it goes away

This is one place a comment's spelling is actually important, because it's here for use in grep 🙂

Oops, missed it! Thanks for catching it.

nit: let's have the same method be used in both of the places we deserialize a ChannelPostPolicy — so not only in the event but in ZulipStream too

For examples, git grep -B2 -A20 ^enum lib/api/, and pick one of the other enums that has a fromRawString or similar method and look at where that enum is used.

… Hmm, there's actually a shortage of enums that work quite like this. (Where it's a numeric value, rather than a string that's the enum value's name, and where at the same time we have a named method we call to deserialize it rather than always doing so as a field on a JsonSerializable class.)

Our enum handling in general feels a bit messy — and I think that's largely because json_serializable is a bit messy in its API for enums — but this particular one is fine for now. Later sometime we can try to find a nicer pattern and sweep through cleaning them up.

Similar to #880 (comment) : canRemoveSubscribersGroup and its old name should have int values, not int?, because those properties are never null on servers new enough to have them.

(OTOH streamWeeklyTraffic can be null on a stream/channel: https://zulip.com/api/get-streams#response so its value in this event can potentially be null too.)